Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(exporter-logs-otlp-http): set useHex to true #3875

Merged
merged 7 commits into from
Jun 21, 2023

Conversation

Nico385412
Copy link
Contributor

@Nico385412 Nico385412 commented Jun 8, 2023

Which problem is this PR solving?

When we use the OTLPLogExporter to send logs to a collector I face this issue:

readLog.traceId: parse trace_id:invalid length for ID, error found

When I look at what the exporter send, traceId look like this x+wKeY+LpUblmgFbiqXBwQ== which is base64. that a bug we should have an hex.

When I look at OTLPTraceExporter i notice it adds an extra parameter true to this function createExportLogsServiceRequest which disable the mapping to base64.

This PR replicate this behavior to OTLPLogExporter

Short description of the changes

createExportLogsServiceRequest function used in OTLPLogExporter now have an extra parameter true to use hex format

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Has been tested in TDD and i had a single new test

  • convert -> should call createExportLogsServiceRequest with useHex parameter to true

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@Nico385412 Nico385412 requested a review from a team June 8, 2023 09:56
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 8, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@Nico385412 Nico385412 marked this pull request as draft June 8, 2023 09:58
@Nico385412 Nico385412 marked this pull request as ready for review June 8, 2023 10:15
Copy link
Contributor

@llc1123 llc1123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should complete the tests. The current tests are too simple.

In the case of this PR is solving, we should compare the results rather than checking the internal parameters.

Please refer tests in other packages.

experimental/packages/exporter-logs-otlp-http/package.json Outdated Show resolved Hide resolved
@Nico385412 Nico385412 force-pushed the fix/enable-usehex branch from 84c58eb to 616b93a Compare June 8, 2023 11:18
@Nico385412
Copy link
Contributor Author

I think we should complete the tests. The current tests are too simple.

In the case of this PR is solving, we should compare the results rather than checking the internal parameters.

Please refer tests in other packages.

Hi !

I just have a look at other packages and i think no one test the "convert" method.
Since it's just a passthrough i don't know what kind of relevant tests i could implements 😅

Do you have some advice or asserts i should write ?

thank you :D

@llc1123
Copy link
Contributor

llc1123 commented Jun 8, 2023

Do you have some advice or asserts i should write ?

You export some log records and expect the output to be in correct otlp-http format.

Please refer to tests of exporter-logs-otlp-grpc and exporter-trace-otlp-http.

I just have a look at other packages and i think no one test the "convert" method.

You don't need to test the convert method alone. The function is already tested in otlp-transformer.

@Nico385412
Copy link
Contributor Author

Thank you for you for your answer.

I'm really sorry but i'm a bit confused by your comment.

You export some log records and expect the output to be in correct otlp-http format.

You don't need to test the convert method alone. The function is already tested in otlp-transformer

Since i don't need to test convert method, and it's already tested in otep-transformer, how can i export log records and expect the result to be in correct otlp-http format ?

As I understand, you ask me to test twice the createExportLogsServiceRequest behavior 😅

@llc1123
Copy link
Contributor

llc1123 commented Jun 8, 2023

Since i don't need to test convert method, and it's already tested in otep-transformer, how can i export log records and expect the result to be in correct otlp-http format ?

I mean you should:

  1. create a http server or sth to receive the output
  2. create a OTLPLogExporter
  3. call the export method with a prepared test sample and target to the mock server
  4. get the result from the mock server
  5. assert the result to be expected

@Nico385412 Nico385412 force-pushed the fix/enable-usehex branch from 616b93a to ae0c1a5 Compare June 9, 2023 07:35
@Nico385412
Copy link
Contributor Author

Since i don't need to test convert method, and it's already tested in otep-transformer, how can i export log records and expect the result to be in correct otlp-http format ?

I mean you should:

  1. create a http server or sth to receive the output
  2. create a OTLPLogExporter
  3. call the export method with a prepared test sample and target to the mock server
  4. get the result from the mock server
  5. assert the result to be expected

Hi !

I tried to perform the same tests as exporter-logs-otlp-proto does but for no reasons, sinon refuse to stub http module

image

I tried :

  • remove my test and copy paste OTLPLogsExporter.test.ts from exporter-logs-otlp-proto
  • keep only the describe(export) tests
  • adapt the code

So far my new test is exactly the same as the one from exporter-logs-otlp-proto i don't understand why sinon is able to stub http in one module and not another.

To solve this issue i tried :

  • upgrade sinon to 15.0.0 (exporter-logs-otlp-proto has version 15 and exporter-logs-otlp-http 14)
  • debug step-by-step

But i dont know what i miss here :/

@Nico385412 Nico385412 force-pushed the fix/enable-usehex branch from 7bdf6ea to eb5f9a8 Compare June 9, 2023 14:52
@Nico385412
Copy link
Contributor Author

Ok should be good now, in the end the issue was with esModuleInterop option ...

Copy link
Contributor

@llc1123 llc1123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

experimental/packages/exporter-logs-otlp-http/package.json Outdated Show resolved Hide resolved
@Nico385412 Nico385412 force-pushed the fix/enable-usehex branch from eb5f9a8 to e6926df Compare June 9, 2023 15:02
@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #3875 (4c7211a) into main (74393ac) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 4c7211a differs from pull request most recent head 6209f9a. Consider uploading reports for the commit 6209f9a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3875      +/-   ##
==========================================
+ Coverage   92.89%   92.90%   +0.01%     
==========================================
  Files         297      297              
  Lines        8838     8838              
  Branches     1815     1815              
==========================================
+ Hits         8210     8211       +1     
+ Misses        628      627       -1     
Impacted Files Coverage Δ
...ogs-otlp-http/src/platform/node/OTLPLogExporter.ts 100.00% <100.00%> (+16.66%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for fixing this 🙂 Would you mind setting useHex to true in the browser exporter as well (src/platform/browser/OTLPLogExporter.ts)? This way we'll have this bug fixed for good 🙂

@Nico385412
Copy link
Contributor Author

Hi, thanks for fixing this 🙂 Would you mind setting useHex to true in the browser exporter as well (src/platform/browser/OTLPLogExporter.ts)? This way we'll have this bug fixed for good 🙂

Done :D

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks 🙂

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, looks like a test is still failing 👀

@Nico385412 Nico385412 force-pushed the fix/enable-usehex branch 2 times, most recently from a54144e to ba446f1 Compare June 14, 2023 07:04
@Nico385412
Copy link
Contributor Author

Can you try again ? looks weird everything is fine on my laptop :/

image

@pichlermarc
Copy link
Member

Can you try again ? looks weird everything is fine on my laptop :/

The browser tests seem to fail, you can run those with NODE_OPTIONS=--openssl-legacy-provider npm run test:browser.
The reason is that the new tests are using node types that are not available in the browser, so the tests need to be adapted to resemble the tests in https://github.com/open-telemetry/opentelemetry-js/blob/abfe059d72d2a50d993ee7fb5ea50a1c29851a31/experimental/packages/exporter-trace-otlp-http/test/browser/CollectorTraceExporter.test.ts

@erikclairiot
Copy link

Facing same issue in our project, any news on this topic ? Thanks @Nico385412

@Nico385412
Copy link
Contributor Author

@pichlermarc should be good :D

@pichlermarc
Copy link
Member

Looks great, thanks for sticking with us on this one. 🙂
Looks like it's just missing "@opentelemetry/resources": "1.14.0", in devDependencies and we can get this PR merged 🙂

@Nico385412 Nico385412 force-pushed the fix/enable-usehex branch 2 times, most recently from 4d35e5b to 767244c Compare June 21, 2023 11:58
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🙂

@pichlermarc pichlermarc merged commit 5dc0f69 into open-telemetry:main Jun 21, 2023
@cristianmadularu
Copy link
Contributor

Any idea when this fix will be available in a release?
Thanks

@pichlermarc
Copy link
Member

pichlermarc commented Jun 22, 2023

Any idea when this fix will be available in a release? Thanks

Planning to release 0.41.0 next week - it will be in that one. 🙂

@cristianmadularu
Copy link
Contributor

Perfect, thank you! :)

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Jun 26, 2023
@jdt
Copy link

jdt commented Jul 3, 2023

Any news on the 0.41.0 version this will be fixed in? I'm waiting on this one too and I'd like to see if there is anything I can do to help get it out there :-)

@pichlermarc
Copy link
Member

@jdt still waiting for approvals for #3938 - it's the last PR that needs to be merged before we release 0.41.0.

@berktec
Copy link

berktec commented Jul 3, 2023

coincidently we are awaiting this bug fix as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log events sent to collector use incorrect encoding of traceid
8 participants